-
Notifications
You must be signed in to change notification settings - Fork 28
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Store FBC in local directory #144
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #144 +/- ##
==========================================
- Coverage 79.61% 79.10% -0.52%
==========================================
Files 2 3 +1
Lines 157 201 +44
==========================================
+ Hits 125 159 +34
- Misses 20 26 +6
- Partials 12 16 +4
☔ View full report in Codecov by Sentry. |
c040464
to
2f9c96d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On first pass this looks pretty good. I have a couple changes in mind though:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some comments denoted with a star ([*]) - they are informative. Respond to them if you'd like, but they are not blocking comments and may be things we consider later.
@@ -149,7 +177,7 @@ var _ = Describe("Catalogd Controller Test", func() { | |||
|
|||
AfterEach(func() { | |||
By("tearing down cluster state") | |||
Expect(cl.Delete(ctx, catalog)).To(Succeed()) | |||
Expect(client.IgnoreNotFound(cl.Delete(ctx, catalog))).To(Succeed()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[*] Tests that create complex cluster state in order to run benefit from having a simple toggle to turn off cleanup. When using a persistent test fixture, if I break this test and want to look at the envtest
or kind
cluster I ran it against, I want a way to keep the test from removing itself from existence on exit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do AfterEach
blocks run if It
's fail? Genuine question. Not sure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do AfterEach blocks run if It's fail?
As far as I am aware, yes
"github.com/operator-framework/operator-registry/alpha/property" | ||
) | ||
|
||
// LocalDir is a storage Instance. When Storing a new FBC contained in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You only need to do this on updates, not the first write.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good call out. That means this Store implementation is ready for both create and update.
@@ -50,5 +50,5 @@ spec: | |||
- "--health-probe-bind-address=:8081" | |||
- "--metrics-bind-address=127.0.0.1:8080" | |||
- "--leader-elect" | |||
- "--feature-gates=PackagesBundleMetadataAPIs=true,CatalogMetadataAPI=true" | |||
- "--feature-gates=PackagesBundleMetadataAPIs=true,CatalogMetadataAPI=true,HTTPServer=false" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Setting this to false for now so that we don't take forever to create all the resources.
const testBundleTemplate = `--- | ||
image: %s | ||
name: %s | ||
schema: olm.bundle | ||
package: %s | ||
relatedImages: | ||
- name: %s | ||
image: %s | ||
properties: | ||
- type: olm.bundle.object | ||
value: | ||
data: %s | ||
- type: some.other | ||
value: | ||
data: arbitrary-info | ||
` | ||
|
||
const testPackageTemplate = `--- | ||
defaultChannel: %s | ||
name: %s | ||
schema: olm.package | ||
` | ||
|
||
const testChannelTemplate = `--- | ||
schema: olm.channel | ||
package: %s | ||
name: %s | ||
entries: | ||
- name: %s | ||
` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are some common elements between test suites that are starting to emerge that'll probably be better housed in a test_util package. I've reframed from creating the common place in this PR to not increase the scope, but we should look into it in a follow up.
18a2e5d
to
f075286
Compare
bc1035b
to
a2f92c2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple nits, but other than that it looks good to me. Approving but would recommend holding merging until others have a chance to take a look
Expect(err).To(HaveOccurred()) | ||
Expect(os.IsNotExist(err)).To(BeTrue()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: the first check seems redundant, but I'm not sure what would happen if you pass a nil error into os.IsNotExist . Also according to the comments on this function we should be using errors.Is
instead:
This function predates errors.Is. It only supports errors returned by the os package. New code should use errors.Is(err, fs.ErrNotExist).
}, cmpopts.IgnoreFields(metav1.Condition{}, "Type", "ObservedGeneration", "LastTransitionTime", "Message")) | ||
Expect(diff).To(Equal("")) | ||
}) | ||
// TODO: GC doesn't work in evntest, and hence simulating a Storage delete error that causes the deletion to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: prefer to capture this in an issue vs commented code, which will rot
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW, rukpak has a TODO issue creator (I think in a github action), that makes sure that issues are created for any TODO comments that are merged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, what's the GC that we're expecting to happen here? As I understand it, the HTTPServer feature gate will no longer use underlying objects. And we don't expect anything to GC the catalog itself (that's a user-created object from the perspective of catalogd).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@joelanford there's more context here: https://redhat-internal.slack.com/archives/GCV27CWNP/p1692993854012409
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My read is
If:
- I'm using envtest
- I put a finalizer on my object
- I delete my object
Then:
- My object actually gets fully deleted even though it has a finalizer on it
Is that right? If so, that smells funny to me. I'm almost positive that the apiserver (which runs with envtest) is the thing looking at finalizers and deciding when to actually delete the object from etcd. Are you sure there wasn't something else about your test setup that was giving you a incorrect impression?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well so the funny thing that you're smelling is this setup of the MockStore, which is actually not doing the job of signaling an error in deleting the stored content.
// }) | ||
}) | ||
|
||
When("there is an error storing the fbc", func() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we don't have one already, we need to capture an issue for handling the case where:
- We've successfully reconciled a catalog and unpacked it successfully
- The spec changed to something that we could not unpack
At this point, what are we doing with the previous successful state, if anything? Is it still active? If so, how do we convey in the status that "we're having trouble reaching desired state, but the previous state is still active"
dad49d4
to
9c58790
Compare
config/manager/manager.yaml
Outdated
@@ -47,8 +47,12 @@ spec: | |||
- "./manager" | |||
args: | |||
- --leader-elect | |||
- "--catalogs-storage-dir=/var/cache/catalogs" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This ends up being gobbled up by the manager_auth_proxy_patch.yaml
patch. If you actually want to explicitly set this value on the deployment, you need to copy it to that file as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you mean "as well"? I removed it from this file and only included it in manager_auth_proxy_patch.yaml
. Slightly bothered by the fact that the file is called auth_proxy_path
but we're including patches that are not auth proxy related, it should probably be in a different patch file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noticed the manager_config_patch.yaml
file wasn't being used at all, moved this arg and the feature gate arg to this previously unused file. There are other fields that should probably go in the new file too but refrained from doing that in this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, it only has to show up in the patch file right now. That is overriding the base. And yeah this could use some refactoring and simplifying in a separate PR.
4e212bc
to
fc8242d
Compare
9467bbc
to
f56fdd2
Compare
closes operator-framework#113 Signed-off-by: Anik <anikbhattacharya93@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm!
04d3828
closes #113